-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Details Revamp] Remove the Three Dot Menu on Affected Reports #44025
[Details Revamp] Remove the Three Dot Menu on Affected Reports #44025
Conversation
@grgia Similarily to how we discovered Tasks, when removing the Three Dot Menus from affected reports, I've noticed a Three Dot Menu Item that appears only for the Concierge chat and only when you have a Guide assigned, which is the Should we also refactor this into a button appearing in the RHP, in a similar way we handled the Tasks |
cc: @grgia |
@cdOut great catch, looks like a yes. (slack thread for ref) |
@parasharrajat from here, please give this a review now. Thanks! |
shouldEnableNewFocusManagement | ||
/> | ||
{isSmallScreenWidth && shouldShowHoldMenu && ( | ||
<ProcessMoneyRequestHoldMenu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we remove this menu? It shows information about held transactions when users open one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I've readded it.
@cdOut Have we moved all the options related functionality to the details page? |
@cdOut Ok great. Then It will be better to add these changes to that PR and remain focused on the main issue. Currently, the Hold/Unhold button functionality has issues as I mentioned above which will be solved when that PR is created. I am happy to review that PR too. Anyways, I believe these changes are unrelated to the current issue we are solving in this PR. What do you say @trjExpensify? |
Yep, works for me!
*Tom Rhys Jones *
*Expensify*
…On Thu, 4 Jul 2024 at 19:35, Rajat ***@***.***> wrote:
@cdOut <https://github.com/cdOut> Ok great. Then It will be better to add
these changes to that PR and remain focused on the main issue. Currently,
the Hold/Unhold button functionality has issues as I mentioned above which
will be solved when that PR is created. I am happy to review that PR too.
Anyways, I believe these changes are unrelated to the current issue we are
solving in this PR.
What do you say @trjExpensify <https://github.com/trjExpensify>?
—
Reply to this email directly, view it on GitHub
<#44025 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD3246IWJNM4RSGMSVLBD33ZKWIXBAVCNFSM6AAAAABJSQRYSSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBZGQ2DIMJSGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Over to you @cdOut. |
Agreed, I'll move those over there. |
@parasharrajat I've reverted those two commits regarding the hold fix, I'll move it to the other issue. |
src/libs/ReportUtils.ts
Outdated
@@ -2805,7 +2805,7 @@ function canHoldUnholdReportAction(reportAction: OnyxInputOrEntry<ReportAction>) | |||
const canModifyStatus = !isTrackExpenseMoneyReport && (isPolicyAdmin || isActionOwner || isApprover); | |||
const isDeletedParentAction = isEmptyObject(parentReportAction) || ReportActionsUtils.isDeletedAction(parentReportAction); | |||
|
|||
const canHoldOrUnholdRequest = !isRequestSettled && !isApproved && !isDeletedParentAction; | |||
const canHoldOrUnholdRequest = !isRequestSettled && !isApproved && !isDeletedParentAction && !isClosedReport(moneyRequestReport); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs to be reverted c1abbb7
(#44025)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a change made because this check was added into MoneyRequestHeader
and MoneyReportHeader
logic for holds, and since the promoted action hold uses ReportUtils I also added it here for consistency. I would advise against reverting it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cdOut do you happen to know the background for why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grgia On main there was the possiblity to hold expenses which were already closed and it was not handled, hence why the !isClosedReport(moneyRequestReport)
was added.
Again, this was added during one of the merges with main but in the MoneyReportHeader
and MoneyRequestHeader
which are removed here in favor of the promoted actions which use ReportUtils
. For consistency's sake and not removing this fix from the flow of the app I would not revert this one.
@parasharrajat Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency's sake and not removing this fix from the flow of the app I would not revert this one.
If we keep this check, shouldn't we update all of the canHoldOrUnholdRequest
declarations (I see 3, ReportUtils, MoneyRequestHeader, MoneyReportHeader) to include this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh NVM it's early, probably not the MoneyRequestHeader
. So yeah this LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted it just in case and I'll include this change in the PR fixing hold logic, should be good to merge.
cc: @grgia @parasharrajat
@cdOut @parasharrajat let me know when this is ready for a final test build |
Just one commit needs to be reverted and we are good here. |
This reverts commit c1abbb7. revert hold affecting commit
merge main into @cdOut/three-dot-removal
@grgia Ready for your review. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tests well!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/grgia in version: 9.0.5-0 🚀
|
🚀 Deployed to staging by https://github.com/grgia in version: 9.0.5-2 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.5-13 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.6-8 🚀
|
Details
This is a final PR for the Details Revamp task which aims at removing the Three Dot Menu from reports affected by the revamp. It also fixes a bug regarding the Hold promoted action not displaying properly for single expenses.
Fixed Issues
$ #42080
PROPOSAL:
Tests
Book a call
works properly, since it has been removed from the three dot menu in that chat.Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
ANDROID-CHROME.mov
iOS: Native
IOS-NATIVE.mov
iOS: mWeb Safari
IOS-SAFARI.mov
MacOS: Chrome / Safari
CHROME.mov
MacOS: Desktop
DESKTOP.mov